Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax trait bounds on generic parameters #275

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tomkarw
Copy link
Contributor

@tomkarw tomkarw commented Aug 16, 2023

Closes #258.

Currently most impl blocks over types in dashmap use similar trait bounds (e.g., K: Eq + Hash) even when those traits aren't actually "used" in the implementations. This forces users writing generic code over dashmap objects to include the same restrictions in their impl blocks.

By contrast, most standard library crates use the least restrictive bounds possible for each method, even if it's not clear why anyone would want to use those methods without common-sense trait bounds.

In the worst case, this prevents users from deriving traits like Debug on generic types containing DashMaps. The implementation of Debug on DashMap doesn't ultimately use the Eq or Hash implementations on K (although some of the intermediate types it uses also have the spurious bounds), so this issue could be avoided by removing the trait bounds.

This commit removes all spurious bounds, leaving only the necessary constraints on per-method basis.
where
K: Eq + Hash + Send,
V: Send,
S: BuildHasher + Clone + Send,
Copy link
Contributor Author

@tomkarw tomkarw Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite inconsistent with other Send/Sync implementations, they all just require K: Send, V: Send. I'm not quite sure why S: Send is not required in all other cases, or why it's required here.

Also, it would be wise to add Safety comments to Send (Sync) implementations, possibly including rationale why we S: Send/Sync is not required.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there still a willingness accept this type of change? I am happy to go through the existing code base and relax the trait bounds where possible. I'll leave Send/Sync bounds where they are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relax trait bounds on generic parameters
2 participants